recorder: inline mode, so a host can have the rows without keeping the bytes - #95
Conversation
…e bytes A second arrangement beside the two-process one: one dz-recorder process that captures a feed, derives its rows through the same derive() archive mode calls, spools them to disk and loads them. It keeps no datagrams. That contradicts the recorder design's "the archive is bytes, not rows", and the spec says so rather than working around it — a rule written next month cannot be run against traffic nobody kept. Three things bound the loss: the mode is opt-in and archive mode is untouched, every row carries a derivation column saying which mode produced it, and the derivation itself is called rather than reimplemented, with an equivalence test over one synthetic feed as the gate. Rows reach disk on every window, not only when the destination is down. A recovery path that runs only during an incident is one nobody has tested; the row sink holds rows in memory for as long as its age bound allows and inline that memory is the only copy; and windows on disk are what bring the ledger back, so a retry is a replace again. The record path gains no configuration key. config_hash is written into every object as provenance, so a column-store endpoint in that file would make a password rotation change what an archive says produced it — and adding any key would change the hash of every configuration in the fleet. Two files, with the identity coming from the recorder's own so the halves cannot disagree about which host they are. docs/README.md had none of the four recorder documents; it has a section now.
Inline mode derives rows from datagrams it does not keep. Archive mode derives them from an object whose sha256 was checked against its manifest first, and whose bytes are still there to derive again. Both write into the same five tables, and without this column a query cannot tell one from the other. A column rather than an inference. An inline row's object_sha256 is empty, so a reader could in principle test for that — which is the reason not to: an empty digest field is a trap, a query that reads it as verified is wrong in the direction that matters, and nothing about the name object_sha256 warns anybody. DeriveInput carries it and has no Default, so a derivation names its provenance or does not compile, and every row in a batch is stamped from that one value rather than filled in per grain. derive_object states `archive` because it verified the digest itself three lines earlier. In no ORDER BY, on any table, and there is now a test that says so: provenance in a sort key stops the two modes' views of one datagram collapsing under ReplacingMergeTree, so a window loaded both ways doubles and every count over it is wrong. 001 declares the column and 005 adds it by ALTER. 001 is the authoritative table definition — tests/ddl.rs holds the row types against its CREATE TABLE blocks column for column — so a fresh deployment gets it from there, and 005 is for the deployments whose tables already exist, where CREATE TABLE IF NOT EXISTS reaches nothing. Adding a column with a DEFAULT is a metadata change, so this is not a mutation of the hundred-million-rows-a-day table. 004 grants INSERT at table level, not per column, so the loader account needs no further grant.
Mechanical, and nothing changed behaviour. `cargo test -p dz-recorder-load -- --list` names the same 56 tests before and after, which is the check this kind of move deserves: passing tests only say the code still works, and an identical test set says nothing moved out of reach of one. ledger, loader and metrics go into src/lib.rs. Inline mode captures and derives in one process, so it has no objects directory to walk and does not want Loader — but everything downstream of the derivation is the same problem, and a second ledger would be a second answer to *is this loaded* that disagrees with the first in exactly the case that matters: after a crash. record_landed carries the rule that an entry is written when an insert is acknowledged and never when the sink accepted the rows, and that rule is worth having once. The command line, the configuration, the endpoint and the build identity stay in the binary. Each is a statement about this binary — inline mode reads its own configuration, serves its own endpoint on the recorder's port and reports the recorder's build — so exporting them would hand a caller the wrong one. Recorded, record_landed and now_unix_nanos were pub(crate), which reaches neither the binary nor inline mode once the modules live in a library: two targets are two crates.
Ten pull requests landed while this branch was in progress, and one of them — the market data rows work — added three grains, three tables and three migrations. So this is not a mechanical merge, and the parts that were not mechanical are these. `derivation` now covers eight grains, not five. `Event`, `Instrument` and `BookTop` are derived from the same datagrams by the same pass, so a reader joining an event row back to the datagram row it came from has to find the same answer on both; a provenance that stopped at the five original tables would be a column that stops being true exactly where the join gets interesting. `EventInput` carries the field for the same reason `DeriveInput` does — the fold reads a `Source` and cannot see whether the bytes behind it were verified. The migration is `008`. `005` through `007` were taken by market data while this branch held `005`, and the five cross-references in `001` were renumbered with it. `the_provenance_column_is_in_no_sort_key` failed on the merged tree, naming the three grains it could not find, because it read only `001` and those tables are declared in `005`. That is the test doing its job rather than a test to relax: it now maps each grain to the migration that declares it, and scans all four schema files for a sort key that mentions the column. The two golden provenance tests enumerated five vectors by hand and would have passed vacuously over the new three. They now go through an exhaustive `match` on `Grain`, so the next grain fails to compile here instead of shipping rows nobody can attribute — the same discipline the column-store sink already applies when it destructures a batch. The library extraction had to take `config` and `market_data` with it. The pass now reaches into both, and a pass cannot be a library while half of what it calls is not. `lib.rs` says so rather than leaving the boundary looking arbitrary. `rust/Cargo.lock` and four import lists conflicted textually and were resolved by keeping both sides. The whole workspace builds, formats, lints and passes 170 test suites on the merged tree.
It lived in the replay crate because the two callers that needed it were an archive iterator and a round-trip test. Inline mode is a third: a live capture handing datagrams across a thread boundary owns what it carries, and reaching this type through the archive reader would pull a pcapng parser and a decompressor into a path that reads no archive at all. `dz-recorder-replay` re-exports it, so every existing caller reaches it where it always has. 174 test suites unchanged.
…ntly Cargo.toml, lib.rs and four empty modules. dz-recorder-replay is deliberately not a dependency: inline mode reads no archive, and OwnedDatagram now lives in core precisely so this build carries no pcapng parser and no decompressor.
Inline mode puts a derivation behind a live capture, so there is now a place a datagram can be lost that did not exist before. A datagram the deriver never sees is a sequence value nobody delivered, and a sequence value nobody delivered with nothing admitted behind it gets a `publisher` verdict — the recorder's own drop reported as somebody else's fault. That is the failure this module exists to prevent, and it is worse than not measuring at all. So a drop is charged through PendingLoss, the accumulator the capture already uses one layer down. The debt rides on the drop_delta of the next datagram that gets through, which is what that field is defined as. `offer` owes the incoming delta to the accumulator BEFORE it tries rather than after it succeeds: the offer may fail, and a delta that left on a datagram which did not get through is loss the rows never hear about. It never blocks. A capture thread waiting for a slot stops draining its receive queue, and a slow derivation becomes feed loss plus a false publisher finding in every window derived during it. A full ring is a drop and a counter — the staging watermark's rule, applied to datagrams. The slots are pooled. OwnedDatagram::from_recorded allocates, and an allocation per datagram on the capture thread is a regression against a path that costs a copy and a buffered write today. The test asserts it on the buffer addresses the derivation is handed: 200 datagrams through a ring of 4 must come back in at most 4 distinct buffers. One slot is always in the deriver's hand, so a producer sees capacity - 1 free while a datagram is out. That is what makes the borrow safe rather than an off-by-one, and it is written down where a reader will hit it.
…claim The window is the object's replacement and only the object's. `derive` reads a Source to exhaustion and consults one thing outside the window it is given — the preceding trailer, which decides one bit — so the object was never the unit of derivation, it was the unit of storage. This makes a live capture look like a source that ends. The bound is checked before the receive, never after. A window that took a datagram and then found it did not fit would have to hold it over, and a datagram held between two windows is in neither one's tally. Checked first, it stays in the ring and opens the next window instead — asserted on the sequence numbers rather than on a count, so a datagram quietly dropped at the boundary fails the test. The wait is sliced, because the age bound exists for a feed that has gone quiet and a receive that blocked until traffic arrived would never let it fire. CoverageTracker is the archive writer's own, not a second implementation. Two accumulators would be two answers to what a window covered, and they would differ in exactly the cases the coverage row exists to describe: a reset inside the window, a datagram too short to attribute, an instance past the cap. The manifest states what was observed and refuses the rest. `sha256` is empty and `byte_count` is zero because nothing was written and nothing was hashed — a digest over anything else would be a different claim wearing the field name of a claim about an archived object, and a reader checking it would be checking nothing. The window key carries the wall-clock start rather than the sequence number, which restarts at zero every run, and says `live/` so nobody goes looking for an object to fetch. The ring gained a wait/in_hand split: a caller that waits in a loop and then returns the datagram cannot hold a borrow across the loop, because the borrow checker cannot see the previous iteration's is dead. The fixtures are the synthetic publisher's. Hand-built datagrams were tried first and were the wrong fixture: the coverage tracker reads the channel, the sequence number and the reset count at fixed offsets, so a test that writes those offsets itself only tests its own idea of the layout.
One synthetic feed, two paths, the same rows. The argument for deriving in flight is not that it is cheaper — it is that it is the same function as deriving from an object. If it is, a row's provenance is the only thing that changed and every query written against archive-mode rows still means what it meant. If it is not, inline mode is a second analysis wearing the first one's column names. A clean feed and all nine injected faults — a gap, backward motion, a reset, a second publisher, one disappearing, a duplicate, a reordered pair, an over-cap declared length, an unknown schema version — now derive identically through both paths, field for field, on every grain. The comparison erases only `derivation`, `object_key` and `object_sha256`, and it erases rather than skips: a comparison that walked the fields it cared about would ignore any field added later, which is most of this test's value. It compares grain by grain and row by row. Two whole batches printed on failure are hundreds of rows with the difference somewhere inside them, and a gate whose failure nobody can read is a gate that gets deleted. The third test is the one archive mode has no equivalent of: a ring overrun in the middle of a feed must not produce a `publisher` verdict. The loss has to fall between two datagrams that arrived, or nothing reveals it — so the capture runs where it runs in production, on its own thread, bursting far past the ring and then pausing, and the datagram that reopens the feed is the one that admits what was lost. Its gaps come back attributed to the recorder with no unexplained residue. Also lands the spool: rows on disk between the derivation and the column store, one window to a directory, a per-grain digest, fsync once at close, oldest first, a byte budget that evicts rather than blocks, replay of what a crash left, and a ledger entry written only after an insert is acknowledged. Eight tests, all about the failures rather than the happy path.
Inline mode is asked for with --inline-config, and RecorderConfig gains no key. Its config_hash is written into every archived object as provenance, so a destination in that file would make a password rotation change what an archive says produced it — and adding any field changes the hash of every configuration in the fleet. site, recorder and env are not in the second file either: two files can name the same host differently, and then one dashboard's live panel and historical panel describe two recorders that do not exist. Everything is behind a build feature that is off by default, so the recorder still builds with no column-store crate and no HTTP client. The four refusals each name their key. An archive directory configured in inline mode is refused rather than ignored, because a host believed to be keeping bytes it never kept for a second is the worst outcome available here. A spool directory that cannot be opened is refused because the spool is this mode's whole durability. A ledger inside the spool is refused for the reason the loader's may not live inside its objects directory. And --inline-config in a build without the feature fails at startup rather than falling back, because falling back leaves a host in the arrangement nobody chose. Two things this leaves open, both stated rather than papered over. Plan::from_config requires staging_dir and completed_dir to be non-empty, and inline mode refuses them with a value, so the two are mutually exclusive as written: --check in inline mode validates the identity, both files and the destination, but not yet the feeds. Making Plan mode-aware is a refactor of startup.rs and belongs with the runner. --inline-config without --check therefore errors saying recording is not wired yet. Falling back to archive mode was the one thing forbidden.
…posts without a lock Two structural gaps found by building on top of the pieces, not by reading them. Plan::from_config requires the archive directories and inline mode refuses them with a value, so the two were mutually exclusive: --check in inline mode validated the identity, both files and the destination, but not a single feed. A group that was not multicast, a port role claimed twice, an interface that did not resolve — all would have started. On the arrangement that keeps no bytes, which is the one that leaves an operator least to diagnose with. So Plan has two constructors and carries which one made it. for_inline runs every feed check from_config runs — inline mode joins exactly what archive mode joins — and drops only the archive's: no directories, no compression, no staging budget, because those keys are refused outright by the other file. FeedPlan's writer configuration becomes Option, absent in inline mode: a set of directories built for a mode that never opens a writer is a set of directories an operator would reasonably expect to find objects in. The archive runner asserts the arrangement it was handed, so a wiring mistake names itself rather than surfacing as a panic in a feed thread. The identity refusal moved to the plan and is now made once. Inline mode takes site, recorder and env from the recorder's own file, so the check belongs where that file is validated, and an operator reads the same sentence whichever arrangement they run. Arrangement::Inline and for_inline are behind the feature. A default build has no inline mode to be in, and a variant it can never construct is one its dead-code analysis is right to flag. The spool gained a two-phase post. Its `post` held `&mut self` across sink.write_batch, and under the Arc<Mutex<Spool>> the pipeline needs — a derivation thread storing, a posting thread posting — that put an HTTP request inside the lock. A slow destination would then block the store, which blocks the derivation, which fills the ring, which drops datagrams: the exact backpressure chain the design forbids, arriving by a route nobody would look for. take_oldest, record_landed and release let the network call happen with no lock held. record_landed takes a list because a coalescing sink lands earlier windows together with the current one. 181 suites green, both builds clean.
The pipeline: a derivation stage turning windows into rows, a spool between, and a posting stage. Three and not two because write_batch posts synchronously and retries — on the derivation stage a merely slow destination would hold up the next window, fill the ring behind it, and turn a column-store problem into feed loss. The lock is never held across the network. Both stages reach the spool, so it sits behind a mutex; the posting stage takes a window under the lock, releases it, posts, and takes it again to record. That is the whole reason the spool has a two-phase API. Writing the tests found two real holes, neither visible from reading the code. **stop() raced the derivation into abandoning a window.** It set a flag and the derivation checked it at the top of its loop, so a shutdown arriving before the first window was open discarded everything already in the ring — datagrams the capture had accepted and the publisher will not send again, leaving a hole in the rows that nothing in them could explain. The fix is in the signature: stop takes the capture end. Closing the ring is what ends the open window, and an ordering the type system enforces is one no caller can get wrong. The derivation consults no stop flag at all now. **A panic between taking a window and recording it stranded it in flight.** The unwind passed both record_landed and release, leaving a window nobody was holding that the spool would never offer again — it would sit on disk ageing the lag gauge until the process restarted. A restarted stage now releases first: a no-op on the first entry and the recovery on every other. And a stage that panics while stopping gets two more attempts, because the flag otherwise reads as "do not come back" and the final flush is exactly when a window is most likely to be in hand. post_if_due is called once a pass, as the loader calls it: the sink holds rows across windows deliberately, so a feed that has gone quiet would hold its last rows until something else arrived — the opposite of what its age bound is for. So is record_landed, for the spool's own contract: a window whose rows are in the store but whose ledger entry would not write owes an entry rather than an insert, and nothing else retries it.
The recorder README gains a "two modes" section beside the "two capture modes" one it mirrors — capture mode is orthogonal to the arrangement, and that first line now says so. It states the cost without softening it: a rule written next month cannot be run against traffic nobody kept, a derivation bug can be stopped but not corrected, and nothing verified the bytes the rows came from. Then the three things that bound it. The loader's README places itself as the other half of archive mode, BRINGING-UP-A-FEED gains the two files and both --check invocations, and there is a systemd unit following the loader's own layout. No README for dz-recorder-inline. Of fifteen crates under rust/recorder only the binary has one, and the module docs already carry the argument; a second copy is a copy that drifts. Three fixes from reading rather than from writing: scripts/check-public-repo-rules.sh was failing on this tree — a spool test used 10.0.0.1, outside the documentation ranges. That is a gate, and it was red. The unreachable-destination message named DZ_LOADER_CLICKHOUSE_PASSWORD as the only source of the password, while the example, the usage text and the config crate all name DZ_LOADER_CLICKHOUSE_PASSWORD_FILE as the preferred one. An operator following the error would have taken the second-best route. A duplicated cfg attribute, harmless and gone. The metrics endpoint now takes a renderer, so one port can serve two registries: inline mode publishes dz_recorder_* and dz_recorder_inline_* from one process, and two ports would be a second target to configure and a second thing to notice missing. Left for the record, not fixed here: recorder-release.yml builds --features afpacket only, so the released asset cannot run inline mode. The loader's release already carries --features tls, so the precedent exists; whether the recorder's asset should carry the column-store client is a deployment decision rather than a wiring one.
The record path, and with it the mode works end to end. A capture thread per feed offering into a ring the pipeline drains, one metrics port serving both families, and a shutdown that keeps what the capture is holding. pump and drain_and_stop are the archive path's own, unchanged. The ordering they encode — take what the capture already has, then stop it — is the part of this binary least worth rewriting: both live sources report Ended as soon as their stop flag is set, so stopping first discards every datagram the drain threads had queued. Then the pipeline takes the sending end, which is what ends the open window, so those datagrams are derived rather than abandoned. A spool and a ledger per feed, for archive mode's own reason. Two writers sharing a staging directory each see the other's open segment as an orphan and evict it; two pipelines sharing a spool would do the same under a budget neither could account for, and two appending to one ledger would interleave lines into a file that parses as neither. The host's spool budget is divided between the feeds that share the disk, exactly as the staging budget is. The signal handler is now one function used by both arrangements rather than two copies that would drift — including the second-signal exit, which matters more here: the shutdown waits on a destination that can hang for as long as it likes. dz_recorder_inline_* covers the three places this arrangement can be wrong that the other cannot: the ring, the spool, and a stage that stopped. Its own family beside the health tier's, because folding them together would make an archive-mode dashboard show empty panels for series that cannot exist there. The help text on the eviction counter says not to alert on it and names the age gauge instead. The roles a window declares are built from the bindings, not copied from a writer configuration an inline plan does not have. The membership address is written only when the join named one: route discovery means the kernel picked it, and an unobserved value must not become a written one. 182 suites green, both builds clean, and the public-repo check passes.
Running the binary rather than the tests turned up two things in what a deployment pipeline reads. The identity was printed twice — once by the plan and once by the inline summary. Two `site=` lines have an operator working out whether the two files disagree, and the answer is that they cannot: inline mode takes the identity from the recorder's own file and the second file has no key for it. A summary that repeated it invented a question the configuration makes unaskable. And the plan printed `archive rotate=...`, `staging budget=0B per feed` and `staging=- completed=-` in a mode that writes no object. The keys are refused outright, so those lines had an operator reading `--check` for a problem that is the absence of a thing this arrangement does not do — and a staging budget of zero reads like a misconfiguration rather than like nothing being staged. Both are now tested rather than left to the next person to notice.
There was a problem hiding this comment.
🟡 Changes recommended
Inline mode can accept a non-zero inline.spool_max that still yields a per-feed spool budget of 0 (via division by feed count), which would immediately evict every window and silently lose all row durability unless explicitly refused.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an inline recorder mode (feature-gated) that derives rows directly from live capture without retaining datagram bytes, while preserving archive-mode behavior as the default. To keep archive and inline outputs comparable and query-safe, it introduces an explicit per-row provenance column (derivation) and adds an equivalence “gate” test that asserts archive- vs inline-derived row sets match aside from the fields that must differ.
Changes:
- Add
derivationprovenance (archivevslive) across all eight grains + ClickHouse migrations/tests to keep it out of sort keys. - Introduce
dz-recorder-inlinecrate (ring/window/spool/pipeline/metrics) and wire it intodz-recorderbehind--features inline+--inline-config. - Refactor
dz-recorder-loadinto a library + binary so inline mode can reuse ledger/recording semantics.
File summaries
| File | Description |
|---|---|
| rust/recorder/README.md | Documents archive vs inline mode and operational tradeoffs. |
| rust/recorder/dz-recorder/README.md | (unchanged path; not in diff) — |
| rust/recorder/dz-recorder/Cargo.toml | Adds inline/tls features and optional deps for inline mode. |
| rust/recorder/dz-recorder/inline.example.toml | Example inline-mode config file. |
| rust/recorder/dz-recorder/systemd/dz-recorder-inline.service | Systemd unit for inline-mode deployment + safety hardening. |
| rust/recorder/dz-recorder/tests/inline_mode.rs | Binary-level inline-mode refusal/UX tests. |
| rust/recorder/dz-recorder/src/cli.rs | Adds --inline-config CLI flag and usage text. |
| rust/recorder/dz-recorder/src/endpoint.rs | Generalizes metrics serving to accept a render closure. |
| rust/recorder/dz-recorder/src/inline_config.rs | Parses/validates inline config; runs inline --check; routes to inline runner. |
| rust/recorder/dz-recorder/src/inline_runner.rs | Inline-mode runtime wiring: capture → ring → pipeline → spool/ledger → ClickHouse. |
| rust/recorder/dz-recorder/src/main.rs | Routes to inline mode when --inline-config is present; prints mode banner. |
| rust/recorder/dz-recorder/src/runner.rs | Adjusts archive runner for Option<ArchiveWriterConfig> + shared signal handler. |
| rust/recorder/dz-recorder/src/startup.rs | Introduces Arrangement; makes per-feed archive config optional in inline mode. |
| rust/recorder/dz-recorder-clickhouse/db/clickhouse/001_recorder_rows.sql | Adds derivation column to base tables (default archive). |
| rust/recorder/dz-recorder-clickhouse/db/clickhouse/005_recorder_market_data.sql | Adds derivation column to market-data tables (default archive). |
| rust/recorder/dz-recorder-clickhouse/db/clickhouse/008_recorder_derivation.sql | Migration to ALTER TABLE ... ADD COLUMN derivation for existing deployments. |
| rust/recorder/dz-recorder-clickhouse/src/ddl.rs | Registers new migration 008. |
| rust/recorder/dz-recorder-clickhouse/src/sink.rs | Ignores batch-level derivation (per-row derivation is authoritative). |
| rust/recorder/dz-recorder-clickhouse/tests/common/mod.rs | Updates fixtures for derivation field. |
| rust/recorder/dz-recorder-clickhouse/tests/ddl.rs | Asserts derivation exists on all grains and not in any sort key. |
| rust/recorder/dz-recorder-core/src/lib.rs | Re-exports OwnedDatagram. |
| rust/recorder/dz-recorder-core/src/owned.rs | Moves/extends owned datagram rationale (thread handoff use-case). |
| rust/recorder/dz-recorder-e2e/Cargo.toml | Adds inline crate to e2e deps for equivalence gate. |
| rust/recorder/dz-recorder-e2e/tests/depth/mod.rs | Threads Derivation::Archive into event derivation input. |
| rust/recorder/dz-recorder-e2e/tests/inline_vs_archive.rs | New equivalence gate test: archive vs inline rows match (normalized). |
| rust/recorder/dz-recorder-events/src/derive.rs | Threads Derivation through event/book/instrument rows. |
| rust/recorder/dz-recorder-events/tests/book.rs | Updates test input to specify derivation. |
| rust/recorder/dz-recorder-events/tests/derive.rs | Updates test input to specify derivation. |
| rust/recorder/dz-recorder-events/tests/state_key.rs | Updates test input to specify derivation. |
| rust/recorder/dz-recorder-inline/Cargo.toml | New crate manifest for inline mode implementation. |
| rust/recorder/dz-recorder-inline/src/lib.rs | Inline-mode crate docs + module exports. |
| rust/recorder/dz-recorder-inline/src/manifest.rs | Synthesizes manifests for windows (empty digest/size by design). |
| rust/recorder/dz-recorder-inline/src/metrics.rs | Implements dz_recorder_inline_* metrics family. |
| rust/recorder/dz-recorder-inline/src/pipeline.rs | (new; not shown in diff excerpt) pipeline orchestration. |
| rust/recorder/dz-recorder-inline/src/ring.rs | Ring buffer with loss-debt propagation and pooled slots. |
| rust/recorder/dz-recorder-inline/src/spool.rs | Disk spool with budget enforcement, replay, and ledger integration. |
| rust/recorder/dz-recorder-inline/src/window.rs | Window as a Source over the ring with byte/age bounds. |
| rust/recorder/dz-recorder-inline/tests/pipeline.rs | End-to-end pipeline tests with fake sink + panic restart behavior. |
| rust/recorder/dz-recorder-inline/tests/ring.rs | Ring correctness + non-blocking + debt accounting tests. |
| rust/recorder/dz-recorder-inline/tests/window.rs | Window bound/manifest correctness tests. |
| rust/recorder/dz-recorder-load/Cargo.toml | Updates crate description to reflect new library role. |
| rust/recorder/dz-recorder-load/README.md | Notes loader as archive-mode “other half” and references inline mode. |
| rust/recorder/dz-recorder-load/src/endpoint.rs | Switches endpoint to use library metrics type. |
| rust/recorder/dz-recorder-load/src/loader.rs | Exposes types/functions needed by inline mode. |
| rust/recorder/dz-recorder-load/src/lib.rs | New library export surface for config/ledger/loader/metrics. |
| rust/recorder/dz-recorder-load/src/main.rs | Uses library modules instead of local modules. |
| rust/recorder/dz-recorder-load/src/market_data.rs | Sets Derivation::Archive for archive-derived market data. |
| rust/recorder/dz-recorder-replay/src/lib.rs | Re-exports OwnedDatagram from core (compat path). |
| rust/recorder/dz-recorder-replay/src/source.rs | Updates import path for OwnedDatagram. |
| rust/recorder/dz-recorder-replay/src/synthetic.rs | Updates import path for OwnedDatagram. |
| rust/recorder/dz-recorder-rows/src/derive.rs | Adds Derivation to DeriveInput; stamps into all rows/batch. |
| rust/recorder/dz-recorder-rows/src/lib.rs | Re-exports Derivation. |
| rust/recorder/dz-recorder-rows/src/rows.rs | Defines Derivation enum and adds derivation field to all grains + batch. |
| rust/recorder/dz-recorder-rows/tests/column_names.rs | Updates column fixtures and label spelling tests for derivation. |
| rust/recorder/dz-recorder-rows/tests/common/mod.rs | Adds “derive as live” helper path for equivalence-style tests. |
| rust/recorder/dz-recorder-rows/tests/golden.rs | Adds provenance checks and archive-vs-live normalization assertion. |
| rust/Cargo.lock | Adds new crate + optional deps under recorder inline feature. |
| rust/Cargo.toml | Adds dz-recorder-inline to workspace members. |
| docs/README.md | Adds recorder section + inline mode spec/plan links. |
| docs/superpowers/plans/2026-09-08-recorder-inline-mode.md | Implementation plan document for inline mode. |
| BRINGING-UP-A-FEED.md | Adds operator guidance for choosing/pointing to archive vs inline mode. |
Review details
Suppressed comments (1)
rust/recorder/dz-recorder/src/inline_config.rs:401
- Inline mode divides
inline.spool_maxacross feeds later (inline_runner), andspool_max > 0can still produce a per-feed budget of 0 when there are enough feeds. That configuration should be refused here (including under--check) so the operator gets a clear startup error instead of silently evicting every window immediately.
- Files reviewed: 61/62 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…g is refused Two findings from review, both real. **Concurrent scrapes inflated a counter.** The metrics endpoint serves every request on its own thread, and a Prometheus counter cannot be assigned — only advanced — so a counter mirroring a total the stages already keep was sampled by reading it, subtracting, and adding the difference. Two scrapes landing together each read the same value and each added the same difference, leaving a `*_total` permanently wrong by an amount nothing records. A scrape now samples and renders under one lock. The test for it is asserted on the exclusion, not on the inflation, and the first attempt is why. A test that runs many scrapes and checks the total afterwards has to lose a race to fail — the sampling is a dozen atomic reads, so it passed against an implementation with the lock explicitly released. The second attempt held the spool's lock across the wait and so serialised on the wrong mutex, and passed too. This one holds the sampling lock open and requires the other scrape to have waited, which either happened or did not. **A non-zero budget can still leave a feed nothing.** `spool_max` is the host's, divided between the feeds that share the disk, and only the literal zero was refused. Two feeds and a small budget give each of them none, every window is evicted as it is written, and the destination stays empty while the recorder reports itself healthy. Archive mode refuses the same shape with `StagingBudgetTooSmall`; this is that refusal for the arrangement that stages rows. It runs after the plan, because it needs the feed count, and before anything is opened, so `--check` is where a host learns it. Zero is the floor this can check rather than a chosen one, and the doc comment says so: a window's rows are derived from datagrams, so what one costs in bytes is not knowable from the configuration, and a multiple of the window bound would be a number nobody measured.
…honest The default decides how a configuration that says nothing is read, and this inverts it: inline mode is the reading, archive mode is asked for by `--archive`. The argument is not the flag. It is that archive mode requires two directories inline mode refuses a value for, so an archive-mode host whose command line names no mode is refused at startup by key rather than read as a recorder that quietly stopped keeping bytes. There is no third configuration shape. A log line would not have been enough: the restart that changes a host's mode is the moment nobody is reading its log. Three new sections. Why the default belongs on the reading that fails loudly rather than the one that fails as an empty table. What it costs — a fleet-wide edit to every archive-mode command line, classed the way the feed-routes design classes its own, and the build feature moving into the default set because a binary cannot refuse its own default mode. And whether the default should keep the bytes too: decided inline only, rejected for four reasons, the strongest being that a both-mode has to accept the very keys whose refusal makes this default safe. `--archive` mints no vocabulary: it is the `[archive]` section, the `derivation` column's value, the archive crate's name and this design's own word for the arrangement. Inferring the mode from whether those directories carry a value is rejected as the same trap the `derivation` column exists to avoid, one layer up.
…nverts the default Task 11: the ten places the old default was written down, the two refusals that have to name the flag, and the revert that has to kill each of the four tests. Its argument is the spec's; this is the list. The progress marks are verified against the tree rather than against the branch's own commit subjects, and four boxes stay open because grepping for what they name found nothing: - the window manifest's cumulative capture drop totals. `window_manifest` takes them and the only caller passes zeros, so every inline coverage row reports a capture that dropped nothing. The equivalence gate cannot see it: the synthetic feed has no kernel drops, so both paths report zero and agree. - the trailer is never read back from the ledger. It is written, and a run starts at `preceding: None`, so the era anchor the design says stays certain across a restart is uncertain on the first window after every one. - rows derived has no `grain` label, and there is no readable last error where archive mode publishes one. - `Fault::SilentChannel` is the one of nine the gate does not run, and it is the fault whose inline behaviour is least like archive mode's: a channel that goes quiet is found by a window closing on age, which is inline mode's own key. Three bullets were amended before being ticked, each with the reason: the refusals live in `inline_config.rs` and not `startup.rs`; `serve_rendering` was added beside `serve` rather than changing its signature, which is what *no change to archive mode* asked for; and the gate erases three provenance fields and not four, because `byte_count` is a manifest field that no grain carries. Task 9's three binary-altitude tests are not written and are named as the largest gap on the branch. Acceptance criteria 2 and 4 need a host rather than a test and are recorded as open.
The default moves onto the reading that fails loudly. `--archive` is how the arrangement that keeps every datagram is asked for; its absence is inline mode. What holds it is not the flag. Archive mode requires `archive.staging_dir` and `completed_dir`, and inline mode refuses either of them carrying a value, so an archive-mode host whose command line has not changed is refused at startup by key and told to pass `--archive` — that message is the migration instruction as much as the refusal, and it is the only place the instruction is certain to be read. A configuration stating neither shape is refused naming both flags: `InlineConfigError::NotStated`, because there is no defensible spool directory, ledger or destination to invent and a recorder that guessed one would load rows into a database nobody chose. The archive-directory refusal runs first, before the missing-file one, because the command line most likely to arrive here by mistake is an archive-mode host's and the useful answer names the key it wrote. `inline` joins the default feature set. A binary that refused the arrangement its own command line asks for when told nothing would be a binary whose default it cannot honour. `--no-default-features` is now the record-only build — no column-store client, no HTTP client, no row crates — and it can only be in archive mode, so it refuses a command line naming no mode by the feature's name and by `--archive`. What kept the destination out of the record path was never this feature: it is that the capture path never blocks and never parses, and that derivation and posting are off it entirely. Ten places said the old default and all ten moved: the CLI usage and its Args, the dispatch in main, three error messages, `Arrangement`, the manifest, the recorder and loader READMEs, the bring-up guide and its checklist, the example configuration, the systemd unit, and the inline crate's own module doc. Two CI consequences, both wired. The record-only build gets its own clippy and test step, because the feature moving into the default set makes it the only build that compiles the `NotCompiledIn` refusal, and a refusal exercised only where it is never made is no refusal at all. And the released asset now carries the mode for free, features being additive over `--features afpacket` — which settles a deployment question this branch had left open rather than leaving a released binary that cannot run its own default. Also fixes a banned word in a workflow comment: `arm` is out in every sense, and the afpacket cfg blocks are branches.
Five reverts run, each watched fail and restored. Two are worth more than a table row. The defaulted-`InlineConfig` revert does not make the recorder start — it gets as far as an empty `inline.spool_dir` and refuses there. So the mutant is unhelpful rather than silent, and the test asserts the difference: an operator whose command line forgot `--archive` would otherwise be told about a key they never wrote, in a file they never made, on a host they believed was keeping bytes. The default-feature revert is caught by nothing but the manifest assertion. With `inline` out of the default set the whole feature-gated suite is compiled out rather than failed, and the archive-refusal test still passes through its `cfg!(not(feature))` branch. A `cfg!` test disappears in the build it exists to catch, and a suite that shrinks reports success. Also records two things the task needed that its bullets had not predicted: the archive-directory refusal has to run before the missing-file one, and every check in `tests/check_mode.rs` needed the flag — which is this change's fleet-wide cost in miniature, and why the refusal test belongs in that file.
The eleventh place the default was expressed, and the one the inversion's own list missed. `tests/shutdown.rs` spawns the real binary against an archive-mode fixture and waits for a segment to appear in `staging`; read as inline mode it was refused by key on `archive.staging_dir` before binding a socket, so a 0.7-second test became a fifteen-second timeout. It gets `--archive`, because an archive-mode shutdown test that quietly became an inline-mode one would be asserting nothing: the object it waits for is the only thing it proves, and inline mode writes none. The refusal firing there is the refusal working. `cargo test --workspace` did not catch it and cannot: the suite is `#![cfg(feature = "socket-e2e")]`, and a suite behind a feature reports *0 tests* rather than *skipped*, so a green workspace run says nothing about it. The plan now records that under the gap it already had about task 9's binary-altitude tests — same class — with the list of feature combinations to run before pushing. Swept every other place that spawns the binary or writes a recorder configuration: `check_mode.rs` and `inline_mode.rs` already name their modes, the only recorder systemd unit is the inline one and passes `--inline-config`, `dz-recorder-load`'s binary test and unit have no mode to name, and the demo compose file does not run a recorder. Every other `staging_dir` in the tree builds an `ArchiveWriterConfig` directly and never reaches the mode dispatch. `recorder_example.toml` gained the statement in prose. It is the archive-mode configuration an operator copies, and a copy run without the flag is refused — loudly and correctly, but the example should say which arrangement it is.
|
Reviewed on a clean clone. Built and tested what macOS allows; Blocker: the manifest is built from a window nothing has walked yet
Two consequences:
The gate cannot see this. Other findings
|
… more the review found The spec's Testing section asserted an equivalence the tree did not have. The equivalence gate builds its own window — walk, build the manifest, derive — and the derivation stage built the manifest from a window it had not opened yet, so the gate was green over a shape nothing ran. That is corrected here, before the code, along with the design sections the blocker needs: why a window is walked twice, what a single pass writes instead, why an empty window spends no window sequence number, and why a window the spool refused hands on no trailer. The plan's tasks 4 and 7 gain the bullets they were missing and lose the outstanding one about the capture drop totals, which task 12 settles: the capture total is wired from the window's own walk, and the interface total stays the zero archive mode also writes, with the reason moved out of a call site and into the manifest builder.
elitegreg
left a comment
There was a problem hiding this comment.
I read the loss accounting closely, since that is the correctness claim with no archive-mode equivalent, and it is right. offer owes the incoming delta before it tries rather than after it succeeds, undelivered adds the datagram itself on top of what it declared, and the unreachable full-channel branch returns the slot to the free list instead of shrinking the pool silently. I checked PendingLoss on the capture side to confirm the arithmetic saturates. Nothing to change there.
The mode inversion I think is the right call, and for the reason the description gives: one reading fails as silence in a dashboard, the other as a non-zero exit in front of the pipeline that caused it. The thing that most de-risks it is not in the argument at all, it is that ArchiveDirectoryConfigured carries the migration instruction in the refusal text, so an operator who hits it on restart knows the fix without leaving the terminal. Worth keeping that message intact through any later edit.
FeedPlan.archive becoming an Option, stop taking the capture end by value, and for_inline running every feed check from_config runs are all the right shapes.
One finding I would want addressed before this runs on a real feed, and three notes. Leaving this as comments without a verdict.
Schema side looks right to me: metadata-only ADD COLUMN IF NOT EXISTS, no sort key touched, DEFAULT 'archive' true of the existing data so the schema can lead the binary roll, and the argument against inferring provenance from an empty object_sha256 is the correct one.
…a zero delta as proof this host dropped nothing A second reviewer found what makes the two hard zeros at the old `pipeline.rs:345` more than a missing diagnostic: `007_recorder_cross_site.sql` consumes the column. It subtracts `capture_drop_total` over consecutive windows and reads a zero difference as `overflow_free = 1`, which is one of the four conditions in `absence_admissible` deciding whether a site's absence may be used as evidence *about the publisher*. Pinned at zero, every inline host is certified provably overflow-free — so kernel receive-queue overflow here becomes an absence admitted as evidence against the publisher, which is the finding the ring's debt accounting exists to prevent, arriving one layer up through a column nobody read. That also corrects this branch's first attempt at it, which summed the `drop_delta` one window walked. The reader subtracts consecutive rows, so a per-window figure lets a window that dropped less than its predecessor subtract to zero and read as clean: the same defect at lower frequency rather than a fix. `rows.rs`, `007`'s own header and `derive.rs`'s interface-drop delta all define the column as cumulative and never reset, and that is what a window now reports. It needed no new plumbing. The reviewer suggested a shared atomic the derivation stage reads at window close, and the ring's counters already are that: `RingCounters` gains a cumulative `capture_declared`, the existing `dropped` is the ring's own half, and `capture_drop_total` is their sum, read through the `Arc` both ends of the ring hold. `WindowSource` samples it on every call, so the figure a manifest carries is the one that was true at the close. Both halves belong: what the capture declared is what archive mode's writer sums, and what the ring refused exists only in this mode and is exactly the loss `overflow_free` must not certify away. `interface_drop_total` is checked against the same view and is not the same case. It appears in no admissibility gate, and the only verdict it can reach is `upstream`, which exculpates — so a zero withholds an explanation rather than manufacturing one, and archive mode writes the same zero deliberately. A number in one mode only would make the two modes reach different verdicts on identical traffic.
…te runs the ninth fault Archive mode prints its mode line and then the plan; inline mode printed the plan first and the mode arrived inside `config.summary()` further down. The description argues that line is the one thing an operator must not miss — it is what a command line can now get wrong by saying nothing — so an operator scanning two hosts read it in two different places. `INLINE_MODE` moves out of the summary and is printed before the plan in both branches, once. Both tests read the *first line* rather than searching the output, in both files, because `contains` is what let the two orders diverge without anything failing. And `Fault::SilentChannel` joins the equivalence gate's list, which is now all nine. The rustdoc says which half of that fault the gate covers: the derivation half, with one channel among several falling silent partway through. Not the timing half — archive mode finds a quiet channel when a segment rotates on its interval and inline mode when a window closes on age, and one window holding the same datagrams as one segment is this fixture's whole premise. Asserting that wants a fixture with a clock over several windows.
`SilentChannel` was missing from the list for the life of the branch and nothing failed, because a fault absent from a loop is not a fault that fails — it is a fault nobody runs, and that looks identical to a pass. Adding the ninth fault closes the instance; this closes the class. The list the gate runs and the list of every fault there is are kept separately, because a list compared against itself compares nothing, and a `match` nobody calls is what keeps the second one honest: a new `Fault` variant is a compile error there. Dropping a fault from the gate's list now fails `the_gate_runs_every_fault_the_replay_crate_injects` by name.
…had no mutant Six more, each run and each restored. The one worth reading is the fault list: dropping a fault from the equivalence gate left every test passing, because a fault absent from a loop is not a fault that fails. That is how it went missing for the life of the branch, so the list's completeness is now asserted.
|
Thank you — the blocker was real, and worse in one respect than the trace says. All of it is fixed, in nine commits on top of The design and the plan were corrected first, in their own commits ( Blocker: confirmed, and one consequence more than you traced
The fix is that a window is walked twice, and the second walk is not an optimisation anyone should remove later: And the gate now derives the way the stage derives. The two passes live in one place — The test that fails against the old pipeline is 1. The hard-zeroed drop totals — wired, and this half was blocker-class tooDecided, not just plumbed, and the answer changed once @elitegreg pointed at
Tests: 2.
|
nikw9944
left a comment
There was a problem hiding this comment.
Blocking: inline windows are derived against a manifest built before the window is walked, so every row carries a zeroed start_ts/end_ts and a repeating object_key, and each restart replaces the previous run's coverage rows. Two further gaps ship wrong values rather than absent ones — no market-data rows are derived at all, and the capture's drop totals are always 0. Minor comments on the spool's byte budget, the era anchor after a restart, and the new sort-key guard are attached inline.
The mode is no longer a default with a flag over it. Each arrangement is selected by the resource only it can run on — archive mode by the two directories it has always required, inline mode by the file carrying its spool, ledger and destination — and the four combinations are total: both stated is refused, neither stated is refused. That replaces the inversion and the flag with a reading, and the argument the earlier draft rejected the reading by does not carry. The `derivation` column exists because a reader cannot infer provenance from an absent digest; two directories an operator typed are a positive statement, not an absence. The failure it predicted needed a two-case rule — an operator who deletes an `[archive]` section gets the fourth row, which refuses. `--archive` is retired rather than kept as an override. It can only agree with the configuration or be refused by it, and the one power an override adds is the power to resolve a refusal, which is starting a recorder in an arrangement its own configuration contradicts. What it would have bought a deployment pipeline is bought totally by `--check`, which refuses unless exactly one arrangement is stated and prints it on the first line. The cost table for the old default is gone with it: no `ExecStart`, no `ExecStartPre` and no infrastructure repository needs an edit, because an archive host selects its arrangement by saying what it already said. The build feature stays in the default set for a new reason — the arrangement is a property of a host's configuration and the released asset is one asset for the fleet. Also decided here, from the same review: inline mode derives the five transport grains and none of the three market data ones, because deriving them would put a codec on the record path against this design's own decision that nothing there decodes a datagram, and would need an instrument table and a book carried across windows and across a restart. The ask therefore exists in inline mode's own file in order to be refused by name, and the summary states in both arrangements which feeds derive them — an emptiness archive mode shares is not made loud by making inline mode stricter about it than the loader is. And two spool rules the same review found: a window owing a ledger entry is the last thing the budget takes, because its rows have landed and its directory is the only remaining evidence of it; and every byte the spool put on disk is a byte its budget can see, so a store that fails removes what it created.
Task 11 replaced its own mechanism rather than reversing its sentences, and the replaced version is recorded rather than deleted: what survived is the analysis of what silence must mean, what did not is the conclusion that silence should mean a mode. The list of ten places the default was written down becomes a list of the places a flag was, and every one loses it. The selection lands in one place — `Arrangement::selected_by`, compiled in every build, because a build that cannot run inline mode is precisely the build that has to refuse a configuration selecting it. Its two refusals move up out of the second file's error type, since a second reading of the same two keys downstream of the selection is a reading that can disagree with it. A fifth test carries what four hand-written cases cannot: the four shapes are total because the two predicates the selection reads are the two the refusals name, and nothing about four separate tests says so. Task 13 is the third review's four findings. The market data gap is decided as not-derived with the ask existing in order to be refused, and the plan records that this is the one answer in the task that is arguable, with the alternative it was weighed against. The two spool findings and the sort-key guard follow, each landing on its own.
…ag names it The mode was a default with a flag over it. It is now a reading of two predicates: an `[archive]` directory carrying a value, and a second file given. Four combinations, total because both key sets are required with no default and are disjoint — archive, inline, both refused, neither refused. `Arrangement::selected_by` is the one place they exist, and it is compiled in every build: a build without inline mode is precisely the build that has to refuse a configuration selecting it, and a refusal that exists only where it is not needed is no refusal. `Arrangement::Inline` therefore loses its cfg, since that build now constructs the variant in order to refuse it, and `writes_an_archive` becomes one comparison instead of a comparison and a `true`. The two refusals move up out of `InlineConfigError`, and `check_archive_is_not_configured` is deleted rather than moved: its condition is the selection, and a second reading of the same two keys downstream of the selection is a reading that can disagree with it. Their wordings change with their position. `ArchiveDirectoryConfigured` was a migration instruction and becomes `BothArrangementsStated`, naming the key and the file so both statements an operator has to reconcile are on the screen. `NotStated` said "no mode was named, so this is inline mode" and becomes `NoArrangementStated`, because no mode named is no longer a mode. `NotCompiledIn` is now reached by a positive statement rather than by silence, so it stops naming a flag and names the directories instead. `--archive` is retired, not kept as an override. Every command line it would be legal on is one the configuration already decides, and the only power an override adds is the power to resolve a refusal — which is starting a recorder in an arrangement its own configuration contradicts. `a_flag_naming_an_arrangement_is_not_a_flag_this_binary_has` asserts it is a usage error and that USAGE does not teach one, because a flag is cheap to add back and the argument against it is long. The migration is visible in the tests as the edit they did not need: `check_mode.rs`, `shutdown.rs` and `inline_mode.rs` select archive mode with the directories their fixtures already carried, on the command lines they already ran. That is the fleet in miniature, and it is why this replaced the inversion. `the_four_configuration_shapes_are_total_and_two_of_them_run` holds the thing four separate tests cannot: that the two predicates read are the two the refusals name. A fifth predicate later would leave four case tests green over a rule that had stopped being total.
…y tables answer nothing Inline mode derives the five transport grains and none of the three market data ones. That emptiness is not new and is not what this changes: archive mode leaves `event`, `instrument` and `book_top` equally empty for a feed with no `[[market_data]]` entry in the loader's configuration, and the loader defends it there. What inline mode had that archive mode does not is no way to ask and nothing saying so — three permanently empty tables, indistinguishable from a feed nobody published on, and no key an operator could have written to find out. So the ask exists in order to be refused. `InlineConfig` gains the loader's own `MarketDataFeed` rather than a second type — one spelling of which feeds derive market data, so the two arrangements cannot grow two — and any entry is `MarketDataNotDerived` at `--check`, naming the feed, the three tables and archive mode. It is checked before the destination probe, so a host learns it whether or not the column store is up. Deriving them was considered and is a design rather than a task. Market data derivation is a codec walk, and nothing in the record path decodes a datagram — the rule that makes the transport grains trustworthy, since a message a decoder would reject still carries the sequence number whose absence is the finding. In archive mode that walk runs in the loader, off the host, over bytes already stored and already hashed; on a recording process's derivation stage a decoder that refuses costs the window, and with it the transport rows the tier exists for. And `derive_events` builds its instrument table per call, so per window nearly every price message would be refused for an unresolved instrument unless the table, the book and every open snapshot cycle were carried across windows and across a restart — durable state spanning the unit the spool exists to bound. A refusal only reaches an operator who tried, so the summary states it in both arrangements: `market_data=none` beside the inline mode line, and `market_data=loader` beside archive mode's, because there the process that decides is the other one. Not made a required key. That was considered and rejected: it would make inline mode stricter than archive mode about a decision the two make identically, and the equivalence of the two arrangements is what the gate on this design asserts everywhere else. If the un-asked case should be loud, that is a change to the loader too.
…eaves nothing the budget cannot see Two spool findings from the third review, and a third in the guard that was meant to catch a column reaching a sort key. `enforce` selected `windows.keys().next()` with none of `take_oldest`'s filter. Evicting an in-flight window is deliberate and unchanged — those rows may yet land and no ledger entry has been earned. Evicting one that *owes* an entry is not the same thing: its rows are in the store, and its directory is the only thing left that can record that they are, carrying the trailer the next era anchor is checked against. So it goes last, preferred against while anything else remains and taken when nothing does, because a budget that stopped bounding the disk to protect an entry would trade a bounded backlog for an unbounded one. And on a `remove_tree` failure it returned with the window still in the map, so `bytes()` stayed over the budget for ever and the same undeletable directory was chosen on every later pass — the budget stopping bounding the disk from the first failure on. It now goes through `delete`, which takes the window out of the map and moves its bytes to `unreclaimable_bytes`, and *then* stops the pass. The pass still stops, for the reason it always did; what changes is that the next pass tries the next window rather than the same one. `store` could leave a directory on disk and out of `self.windows`, so its bytes sat outside `bytes()`, `enforce()` and `unreclaimable_bytes` alike: the spool reported itself empty while orphans accumulated, one per failed window, until a restart. The fallible part is now `write_window` and `store` is its error path — a `?` in the middle of a function that has already created a directory is a `?` that leaks it, and there were six of them. The byte count is a claim about the disk, and a claim with an exception is not one. The sort-key guard read only the line beginning `ORDER BY`, and three of the eight keys wrap, so `derivation` appended to a continuation line would have passed. It now accumulates a clause until the parenthesis depth it opened returns to zero — which also terminates a bare `ORDER BY` inside a window specification on its own line. The clause count is asserted too: a guard that reads more than one line is a guard whose reading is the thing that can regress, and a walker that quietly stopped finding clauses would leave it green over the hazard it was widened for.
…in both directions Tasks 11 and 13 tick, and their ten reverts are recorded with the test that died under each — committed first, mutated, watched fail, restored. The sort-key one is recorded in both directions, because that is the only way that finding can be shown. The widened guard fails on `derivation` appended to a wrapped sort key; with the walker also reverted to the line the clause starts on, the same hazard **passes**. That is the finding — not that the guard could be better, but that it was green over a column in a sort key. Two weaknesses are stated rather than dressed up. The `market_data=none` summary line is held only by a test asserting a string is printed, which is the weakest shape of gate there is; the refusal beside it is what carries the finding. And nothing asserts that a window merely in flight is still evicted, because a test asserting the absence of a condition passes for as long as nobody adds it.
|
@bgm-malbeclabs — finding 5 accepted and landed. The mode is inferred from the configuration; Six commits on The mechanism, and the part of your argument I carried into the documentYour rebuttal is the load-bearing sentence and it is now the design's:
That is exactly the join the old analogy did not make. The column replaces a reading of a hole a writer left, which a reader can only interpret by knowing how the writer behaved. Two directories in a configuration file are a statement by the person who took the decision, in the file that is that decision's record. Reading them is not inference; it is reading. And the failure the old analogy predicted — an operator deleting an
Total, because each arrangement requires what the other has no use for and no member of either key set is defaulted. So there is no silence left over for a default to be placed on, and that operator gets a non-zero exit code before a socket is bound rather than a mode they did not choose. One detail I had to decide that your sketch left open: either directory states an archive, not both. A half-stated archive is a complete statement about the arrangement and an incomplete one about the archive, so it selects archive mode and is then refused by key. Read the other way, an operator who wrote
|
bgm-malbeclabs
left a comment
There was a problem hiding this comment.
The code reads correct. The description is not the code.
The body describes a design this branch removed
The body says the mode is named by a --archive flag, that inline is the default, and that "every unit, pipeline and runbook that starts dz-recorder for archive mode needs --archive". None of that is on the branch.
8fb68c8 recorder: the arrangement is what the configuration states, and no flag names it deleted the flag. cli.rs parses --config, --inline-config, --check, --run-for, --help and --version, and nothing else. Arrangement::selected_by reads the two [archive] directories and whether a second file was given, and returns NoArrangementStated when neither is stated. dz-recorder-inline/src/lib.rs puts it in one line:
Neither arrangement is a default ... So silence is neither arrangement.
So the body's mode table is wrong in four rows of five, its "Five reverts" table names two tests about a flag that does not exist, and the paragraph saying that inferring the mode from the directories "was considered and rejected as the same trap the derivation column exists to avoid" describes the opposite of what shipped.
This is not only tidiness. A reviewer reading the body concludes that this PR breaks every archive-mode unit file in the fleet, and blocks it on work in infrastructure repositories this one does not contain. The branch needs none of that.
The open-items list is stale too. "window_manifest takes them and its only caller passes 0, 0" was fixed in cd39166. The function has no such parameter now and reads tally.capture_drop_total off the ring's counters.
The new crate does not build off Linux, for one u32
dz-recorder-inline depends on dz-recorder-capture for PendingLoss, which is pub struct PendingLoss { owed: u32 }. dz-recorder-capture does not compile on macOS: nix's ControlMessageOwned has no ScmTimestampns, RxqOvfl or Ipv4Ttl there. So cargo test -p dz-recorder-inline fails at the dependency, and I could not run one of the 2,400 lines of tests this PR adds.
The Cargo.toml already makes this argument against a different crate:
dz-recorder-replayis deliberately absent. Inline mode reads no archive, and depending on it forOwnedDatagram— which is why that type moved todz-recorder-core— would have pulled a pcapng parser and a decompressor into a build that opens neither.
Same move, same reason. Put PendingLoss in dz-recorder-core and the crate and its tests run on any machine, which is what a gate this important should not need a Linux host to run.
Three smaller ones
pipeline.rs carries a private now_unix_nanos. spool.rs, in the same crate, imports dz_recorder_load::now_unix_nanos. The two disagree on overflow: the shared one truncates with as u64, the private one saturates to u64::MAX. Delete the private copy.
spawn_stage retries without bound while running. remaining_after_stop only counts down when stop is set. A stage that panics on every pass restarts for ever at full speed and writes a line to stderr each time. Nothing paces it.
Pipeline::stop waits on that loop. It joins the derivation handle before it sets stop, so a derivation caught in the loop above never lets shutdown reach the posting stage. Setting stop before the first join costs nothing: the derivation still ends on the dropped sender rather than on the flag.
What I read, and what I could not run
ring.rs, window.rs, manifest.rs, spool.rs, pipeline.rs, derivation.rs, inline_config.rs, startup.rs, main.rs, cli.rs, 008_recorder_derivation.sql.
The loss accounting holds up. The debt is owed before the offer rather than after it succeeds, capture_drop_total sums both summands and is cumulative, and Offered::Disconnected is separated from Dropped for the right reason. The manifest leaving sha256, byte_count and interface_drop_total empty rather than plausible is the right call and is argued where a reader will find it. The migration adds a column with a DEFAULT and touches no ORDER BY, which is the cheap and correct shape.
No test ran here. See above.
…a hole the sequence exists to show The two review threads at pipeline.rs:162 and :163 are one finding, and the decision they were waiting for is that the ledger's trailer is not read back. The predecessor test is segment_seq + 1 and a run begins at zero, so a previous run's trailer precedes nothing: the read on its own changes no answer anywhere, and the version that does changes it by claiming the derivation was not down over the one interval in which it was. The spec's *The era anchor gets better, not worse* now says where it stops and answers the three questions the decision needed — absent against unreadable, which trailer the ledger actually holds, and why a wrong anchor is worse than an uncertain one. Task 4's open item becomes task 14's decision.
…or on describes a run the capture stopped after `window_seq: 0` and `preceding: None` keep their values and gain their reason. The predecessor test is `segment_seq + 1` and a run begins at zero, so the ledger's trailer precedes nothing in the run that reads it: wiring the read alone changes no answer anywhere, and the version that continues the sequence so that it would tells a reader the derivation was not down over the one interval in which it was. `007_recorder_cross_site.sql` is what pays for that — the capture-drop counter belongs to the capture handle, and a delta of zero over a handle opened seconds earlier reads as a host that admitted nothing. `NoLedger` loses the clause promising an era anchor certain across a restart, which nothing keeps. The ledger is still required for the reason that is true. `a_restart_does_not_anchor_its_first_window_on_the_ledgers_trailer` runs two pipelines over one spool directory and one ledger, asserts the trailer is there to be read, and asserts the run that starts under it still numbers its first window zero and still writes an uncertain anchor.
…killed nothing The mutation the two threads name — the trailer read on its own — leaves the suite at 1457 passing, this task's own test included. The one beside it, which also seeds the window sequence, kills that test on both assertions and prints what it writes instead: an era row carrying anchor_certain 1 and continuation 1 over an interval in which nothing was captured.
…s captured
`dz-recorder-inline` linked `dz-recorder-capture` for exactly one type:
`PendingLoss`, which is `{ owed: u32 }` and touches nothing else. That
dependency does not build off Linux — `dz-recorder-capture` reads
`ScmTimestampns`, `RxqOvfl` and `Ipv4Ttl` out of nix's `ControlMessageOwned`,
and none of the three exists on macOS — so `cargo test -p dz-recorder-inline`
failed at the dependency and the crate's tests could not be run on a
developer's machine at all.
The argument for moving it is the one this crate's own Cargo.toml already
makes about `dz-recorder-replay` and `OwnedDatagram`: a crate that opens no
pcapng should not link a pcapng parser to name a type. This crate captures
nothing. It is handed datagrams and accounts for what it could not take, and
`PendingLoss` is that accounting, so it belongs in `dz-recorder-core` beside
`RecordedDatagram::drop_delta` — which is what its doc comment is about.
`dz-recorder-capture` re-exports it, as `dz-recorder-replay` re-exports
`OwnedDatagram`, so a capture still reads it where it reads everything else.
`dz-recorder-inline` no longer names the capture crate at all: `cargo tree -p
dz-recorder-inline -e normal,dev` contains neither it nor `pcap`. `nix` still
appears there through `ctrlc`, which is cross-platform.
The macOS build is not verified here — this host has no darwin target
installed — so what is claimed is the closure, which is checkable, and not
the compile.
No test moves and none is added: the workspace suite is 1457 passing before
and after. Gates: fmt; clippy on the default set, `--no-default-features`,
`-p dz-recorder-capture --features afpacket` — which is where the moved
import had to be fixed, since the default set does not compile that file —
and `-p dz-recorder-e2e --features socket-e2e`.
…fore the join it bounds Three findings on the inline pipeline, all in `pipeline.rs`. **One wall clock.** `pipeline.rs` carried a private `now_unix_nanos` while `spool.rs`, in the same crate, imported `dz_recorder_load::now_unix_nanos`. The two disagreed on overflow: the private one saturated, the shared one truncated with `as u64`. The private copy is deleted and the surviving one takes the saturating body, so nothing is lost by the deletion — a wrapped stamp is a row dated inside the sequence space of every other row, which is an ordering error rather than an out-of-range one and the harder of the two to see. **A restart is paced.** `remaining_after_stop` counts down only while the stop flag is set, so a stage that panicked on every pass while *running* restarted at the speed of the panic: a busy loop taking the CPU the capture needs, writing a line to stderr per pass and burying the first one — the only one that names the bug. The wait now doubles from `RESTART_DELAY` to `RESTART_DELAY_MAX`, and a pass that ran longer than the ceiling resets it, so a stage that panics once an hour is never slowed. The shutdown path is deliberately not paced: it is bounded by the count instead, and sleeping between attempts already given up on spends a supervisor's stop timeout. **Each stage's flag is set before the join that waits on it**, which is why there are now two. `stop` set one flag *between* the two joins, so a derivation inside the restart loop was joined for ever — nothing counted down, and the shutdown never reached the posting stage. That one flag could not simply be moved earlier: setting it before the derivation's join would tell the posting stage to make its last pass while the derivation was still spooling into it, which is the ordering `stop` exists to get right. `derivation_stage` reads its flag nowhere — the ring closing is its ending — so the split costs nothing on the ordinary path. Three tests, on `spawn_stage` directly, since the stages themselves offer no panic injection point: every spool lock in this file is `unwrap_or_else(PoisonError::into_inner)`, so a panicking sink cannot make a second stage panic. * `a_stage_that_panics_immediately_is_paced_before_it_is_begun_again` — three panics owe the floor, twice it and twice that. Remove the sleep and it finishes in microseconds and fails; the restart *count* is identical either way, which is why the assertion is on elapsed time. * `a_stage_that_panics_on_every_pass_ends_once_its_flag_is_set` — waits for the thread rather than joining it, so a regression fails the test instead of timing the suite out with nothing in the log naming it. * `giving_up_on_a_stopping_stage_costs_no_delay`. **What has no test is the ordering itself.** Putting the flag back between the joins passes every test in the workspace; failing it needs a panicking derivation, which nothing here can produce. The unit test above pins the half that is reachable — the loop ends only while its flag is set. Recorded here rather than left for a reviewer to find by running the revert. Gates: fmt; clippy on the default set, `--no-default-features`, `--features afpacket`, `-p dz-recorder-e2e --features socket-e2e`. `cargo test --workspace` 1457 passing before, 1460 after.
…eted `8fb68c8` removed `--archive` and made each arrangement selected by the configuration only it can run on. Three comments did not follow it, and each tells a reader the opposite of what the code does. `recorder_example.toml` contradicted itself inside seven lines: "there is no flag" at the top, "The flag is not optional" below it. It now says what selects an arrangement and what a host in the other one leaves unstated. `tests/inline_mode.rs`'s module docs called inline mode the reading of a command line naming no mode, and described two of its own tests as being about a default. Neither arrangement is a default; the tests are about the selection, and about `inline` being in the default feature set because one released binary serves hosts in both arrangements. `dz-recorder/Cargo.toml`'s `inline` feature said "a command line naming no mode" fails without the feature. Every command line names no mode now. The refusal fires on a configuration stating the inline arrangement, which is what it says. Comments only, and no behaviour: 1460 passing before and after. The assertions that keep the flag gone — `a_flag_naming_an_arrangement_is_not_a_ flag_this_binary_has` and the `USAGE` check — were already there and are untouched.
|
@bgm-malbeclabs — all four, on The body described a design this branch removedRewritten. The mode table is now the four shapes
"What it costs existing configurations" now says: nothing. You were right that this was not tidiness. An archive-mode host already states the two directories, which is the whole of the statement that selects archive mode, so no unit file and no infrastructure repository changes before this merges. The paragraph claiming otherwise was blocking the PR on work that does not exist. The Three comments in the tree had the same problem, and The crate builds off Linux
The dependency is gone entirely, not just the import: Not verified: the macOS compile. This host has no darwin target installed. What I can claim is the closure, which is checkable from either machine; the compile is yours to confirm, and I would rather you did than that I asserted it. The The three smaller ones
Three tests, on
What has no test is the ordering itself, and I would rather say so than have you find it. Putting the flag back between the joins passes every test in the workspace — I ran that mutation. Failing it needs a panicking derivation, which nothing here can produce. The unit test above pins the half that is reachable: the loop ends only while its flag is set. If you want a panic injection point in Gates
|
nikw9944
left a comment
There was a problem hiding this comment.
lgtm — re-checked at 690fb99. The era-anchor finding is closed in both halves: the NoLedger overclaim is corrected, and not reading the trailer back is verified correct rather than deferred, since precedes is segment_seq + 1 against a run starting at zero, so the read is inert and forcing it would manufacture a false overflow_free.
…aken Rebased onto a main that now carries #95, which added `dz_recorder_rows:: Derivation` -- the row's Archive/Live provenance column -- and imported it into `derive.rs`, the same file this adds its state type to. Two `Derivation` names in one module is `error[E0255]`, so the new type is renamed rather than the established one aliased: it is the one that is new here, and the row's column is used across the workspace. The rename is mine only. `dz_recorder_rows::Derivation` at derive.rs:35 and the `EventInput::derivation` field at :77 are untouched. `EventInput` gained that field with no default -- a derivation states its provenance or does not compile -- so tests/split.rs now sets it. `Archive` on both sides of every comparison, because the property under test is that splitting changes nothing and the field is the caller's statement rather than the fold's finding. The collision was found by CI rather than by review: the merge ref for a PR is built against the base branch already merged into main, so it compiled the tree this will land as while the branch alone still built clean. Gates on the rebased tree: fmt clean, clippy clean on CI's stable, 1553 passing and 0 failing, and the public-repo rules.
…aken Rebased onto a main that now carries #95, which added `dz_recorder_rows:: Derivation` -- the row's Archive/Live provenance column -- and imported it into `derive.rs`, the same file this adds its state type to. Two `Derivation` names in one module is `error[E0255]`, so the new type is renamed rather than the established one aliased: it is the one that is new here, and the row's column is used across the workspace. The rename is mine only. `dz_recorder_rows::Derivation` at derive.rs:35 and the `EventInput::derivation` field at :77 are untouched. `EventInput` gained that field with no default -- a derivation states its provenance or does not compile -- so tests/split.rs now sets it. `Archive` on both sides of every comparison, because the property under test is that splitting changes nothing and the field is the caller's statement rather than the fold's finding. The collision was found by CI rather than by review: the merge ref for a PR is built against the base branch already merged into main, so it compiled the tree this will land as while the branch alone still built clean. Gates on the rebased tree: fmt clean, clippy clean on CI's stable, 1553 passing and 0 failing, and the public-repo rules.
A second arrangement beside the two-process one: one
dz-recorderprocess that captures a feed, derives its rows through the samederive()archive mode calls, spools them to disk and loads them. It keeps no datagrams.Neither arrangement is a default, and no flag names one. Each is selected by the configuration only it can run on, and a configuration stating both or neither is refused before a socket is bound.
Design:
2026-09-08-recorder-inline-mode-design.md· Plan:2026-09-08-recorder-inline-mode.mdWhat it costs, said first
This contradicts the recorder design's "the archive is bytes, not rows", and the spec argues that rather than working around it. A rule written next month cannot be run against traffic nobody kept. A derivation defect found later can be stopped but not corrected. Nothing verified the bytes the rows came from.
Three things bound it, and each is enforced rather than asserted:
archive.staging_dirandcompleted_dir; inline mode requires--inline-configand refuses either directory carrying a value. Each mode is named by what it cannot run without, so there is nothing to keep in step with the configuration and nothing that can disagree with it.derivationcolumn on all eight grains — in noORDER BY, so deduplication is untouched.How an arrangement is selected
Arrangement::selected_byreads two statements — whether the[archive]directories carry a value, and whether a second file was given — and the four shapes are total:[archive]directories--inline-configBothArrangementsStated, naming the archive key and the fileNoArrangementStated, naming both statementsThere is no row where a host that wanted an archive gets a running recorder without one, and none where a host that wanted rows gets an archive. A log line saying
mode=inlinewould not have been enough: the restart that changes a host's arrangement is the moment nobody is reading its log, and the finding would arrive weeks later as a year of retention that was never kept.Silence is neither arrangement, not a default. Archive mode needs two directories, inline mode needs a spool, a ledger and a destination, and not one of the five has a defensible value to invent — a recorder that guessed a destination would load rows into a database nobody chose. So state nothing is a refusal rather than a reading.
A flag was tried first and removed. Selecting the mode with
--archivemeant a statement on the command line that had to be kept in step with a statement in the file, and two statements of one fact are a pair that can disagree — on the restart where nobody is watching.a_flag_naming_an_arrangement_is_not_a_flag_this_binary_hasand aUSAGEassertion keep it gone.--checkis where a host learns which arrangement it is in: it prints the mode on its first line and exits non-zero if the configuration states none or both.It is inline only, never inline plus an archive. Decided in the spec, with four reasons; the strongest is that a both-mode would have to accept the archive keys whose refusal is what keeps an inline host from silently ceasing to keep bytes.
What it costs existing configurations
Nothing. An archive-mode host already states
[archive] staging_dirandcompleted_dir, which is the whole of the statement that selects archive mode, and it passes no second file. Its unit files, pipelines and runbooks need no edit, and no infrastructure repository this one does not contain has to change before this merges.inlinejoins the default feature set, because one released binary serves hosts in both arrangements. A configuration stating the inline arrangement in a build without the feature is refused at startup naming it, exactly ascapture.mode = "afpacket"is for its own — falling back to archive mode would leave a host keeping bytes where rows were asked for.--no-default-featuresis now the record-only build: no column-store client, no HTTP client, no row crates, archive mode only. What kept the destination out of the record path was never that feature — it is that the capture path never blocks and never parses, and that derivation and posting are off it entirely.The gate
dz-recorder-e2e/tests/inline_vs_archive.rsfeeds one synthetic feed through both paths — recorded to a real archive and derived withderive_object, and pushed through the ring and a window and derived withderive— and asserts the row sets are equal but forderivation,object_keyandobject_sha256. A clean feed and every fault the replay crate injects pass,Fault::SilentChannelincluded, andthe_gate_runs_every_fault_the_replay_crate_injectsis what stops one being added to the crate and forgotten here.It erases those three fields rather than skipping them, so a field added later is compared without anyone remembering to add it. And it compares grain by grain and row by row: two whole batches printed on failure are hundreds of rows with the difference somewhere inside, and a gate whose failure nobody can read is a gate that gets deleted.
The correctness requirement that has no archive-mode equivalent
A datagram the derivation never sees is a sequence value nobody delivered, and a sequence value nobody delivered with nothing admitted behind it gets a
publisherverdict — this recorder's own drop reported as somebody else's fault.So the ring charges its drops through
PendingLoss, andofferowes the incoming delta before it tries rather than after it succeeds: the offer may fail, and a delta that left on a datagram which did not get through is loss the rows never hear about. A third test in the gate overruns the ring in the middle of a live feed and asserts the resulting gaps come back attributed to the recorder with no unexplained residue.PendingLosslives indz-recorder-core, not in the capture crate. This crate captures nothing — it is handed datagrams and accounts for what it could not take — and depending ondz-recorder-capturefor oneu32pulled in nix control messages that do not exist off Linux, so the crate and its 2,400 lines of tests could not be built or run on a developer's machine. Same move asOwnedDatagram, same reason.cargo tree -p dz-recorder-inline -e normal,devnames neither the capture crate norpcap.Rows reach disk on every window, not only during an outage
Four reasons, and the fourth decides it: a recovery path that only runs during an incident is one nobody has tested; a crash otherwise loses whatever the row sink was holding in memory, and inline that memory is the only copy; the recording process's memory stops depending on the destination's health; and windows on disk bring the ledger and its idempotence back.
Configuration
RecorderConfiggains no key. Itsconfig_hashis written into every archived object as provenance, so a destination in that file would make a password rotation change what an archive says produced it — and adding any field changes the hash of every configuration in the fleet.site,recorderandenvare not in the second file either: two files can name one host differently, and then a dashboard's live panel and historical panel describe two recorders that do not exist.Inline mode's own keys are in that second file: the window bound, the ring, the spool and its budget, the ledger, and the destination.
Five refusals, each naming its key or its file: an archive directory configured beside the second file, an unwritable spool, a ledger inside the spool, a configuration stating neither arrangement, and the inline arrangement asked for from a build that does not carry it.
What building it turned up
Each of these was found by writing the next piece, not by reading:
stop()raced the derivation into abandoning a window. It set a flag the derivation checked at the top of its loop, so a shutdown arriving before the first window was open discarded everything already in the ring. The fix is in the signature:stoptakes the capture end, and an ordering the type system enforces is one no caller can get wrong.Plan::from_configand inline mode were mutually exclusive, so--checkvalidated the identity, both files and the destination but not a single feed — on the arrangement that leaves an operator least to diagnose with.Spool::postheld its lock across the insert, which under the pipeline's mutex is a slow destination blocking the derivation, filling the ring and dropping datagrams. Split into a two-phase API so the network call happens with no lock held.scripts/check-public-repo-rules.shwas failing on an address outside the documentation ranges.Verification
1460 tests, 0 failures.
clippyandfmtclean on the default set, on--features afpacket, on--no-default-featuresand on-p dz-recorder-e2e --features socket-e2e; the public-repo check green.cargo test -p dz-recorder --no-default-featuresis the record-only build and has its own CI step, because the feature moving into the default set makes it the only build that compiles theNotCompiledInrefusal. Every test in the new crate runs with no socket, no privileges and no server — and, sincePendingLossmoved, with no Linux either.Every revert was run — reverted, watched fail, restored — and each task's table is in the plan rather than summarised here, because the list is now long enough that a summary would be the thing that goes stale. The two worth naming:
derivationappended to the continuation line of005_recorder_market_data.sql:140provenance_is_on_every_grain_and_in_no_sort_keypreceding: ledger.trailer().cloned()— the fix two review threads asked fora_restart_does_not_anchor_its_first_window_on_the_ledgers_traileron both assertionsNot verified here: a run against a live feed and a live column store. The tests cover the path with the synthetic publisher and a fake sink, and
--checkreaches the destination probe, but a--run-for 60sagainst real traffic has not been done. The macOS build is not verified either — the claim is the dependency closure, which is checkable, and not the compile.What the plan records as still open
rows_derivedhas nograinlabel, and there is no readable last-error string where archive mode publishes one.--run-forrun posting with no server, the same against a real server, and the kill-and-restart durability case — are not written, and acceptance criteria 2 and 4 need a host rather than a test.Two items left this list during review rather than being closed quietly. The window manifest's capture drop totals were hard-zero and now read
tally.capture_drop_totaloff the ring's counters. And the trailer not being read back from the ledger was an open item; it is now task 14's decision — a run starts atwindow_seq: 0andpreceding: Nonedeliberately, because a trailer left by the previous run precedes nothing in this one, and the version that would make it precede something asserts a continuation across an interval in which nothing was captured.NoLedger's refusal no longer promises an anchor certain across a restart, because that is not going to become true.